Add GitHub Actions CI with a PHP version matrix - #6
Merged
Conversation
TomK
force-pushed
the
ci-actions-matrix
branch
2 times, most recently
from
September 3, 2026 13:56
f5136ac to
c68bd75
Compare
#3 tightened the standard-port comparisons from == to ===, in both Request::isStandardPort() and LinkBuilder::_isStandardPort(). Neither receives a reliably-typed port, so the strict comparison silently stopped matching: - LinkBuilder::setPort() is documented "mixed" and callers pass strings. setPort('443') on an https URL emitted https://host:443/path instead of https://host/path. setPort(443) worked, which is why only one assertion in LinkBuilderTest caught it. - Symfony's Request::getPort() returns int|string|null. With no HOST header it returns SERVER_PORT verbatim, which the SAPI provides as a string, so isStandardPort() returned false for a genuinely standard port and urlSprintf("%o") appended ":80" / ":443" to generated URLs. Cast to int before comparing, in both places. The strict comparison stays, so a non-numeric port still does not count as standard. Adds coverage for the string-typed port on both paths -- LinkBuilder via setPort('80') and Request via SERVER_PORT with the HOST header removed. Both new assertions fail without this change. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Adds a setup-php matrix over 8.2, 8.3, 8.4 and 8.5, with fail-fast disabled so
every leg reports independently. This repo had no CI of any kind.
The matrix starts at 8.2 rather than dal's 8.0. composer.json claims
"php": "^8.0", but symfony/http-foundation ^v7.2.0 resolves to 7.4.x, which
requires PHP >= 8.2, so composer cannot install on 8.0 or 8.1 -- verified by
those legs failing on an earlier push of this branch. Bumping the declared
floor to ^8.2 would make composer.json honest, but that is a packaging
decision and is left alone.
Note that the suite is red on master: LinkBuilderTest::testAsUrl expects
setPort('443') on an https URL to drop the default port, and asUrl() now
emits it. That predates this change and reproduces on every PHP version in
the matrix, so it is left for a separate fix.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
TomK
force-pushed
the
ci-actions-matrix
branch
from
September 4, 2026 08:56
c68bd75 to
543cd70
Compare
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This repo had no CI at all — no Actions, no CircleCI. Adds a
setup-phpworkflow matchingpackaged/dal's shape, withfail-fast: false.Two things came out of getting it running, both pre-existing:
The matrix is 8.2 only, not dal's 8.0–8.2. composer.json says
^8.0, butsymfony/http-foundation ^v7.2.0resolves to 7.4.x, which requires PHP >= 8.2. I pushed 8.0 and 8.1 legs first and both died atcomposer install. The declared floor is already false; bumping it to^8.2is the honest fix but it's a packaging call, so composer.json is untouched.packaged/routing(#7) hits this through the same chain.The 8.2 leg is red, and that's the point.
LinkBuilderTest::testAsUrlexpectssetPort('443')on anhttpsURL to drop the default port;asUrl()now emitshttps://secure.cubex.com:443/order. With no lockfile the transitive resolve moved under the repo and nothing was watching. EitherLinkBuildershould suppress default ports again or the expectation is stale — a separate change. Merging the workflow is what makes it visible.Test plan
Fresh
composer installplus the full suite on PHP 8.2.33 from a clean checkout of master: 43 tests, 136 assertions, the one failure above and nothing else. Same single failure with the in-progresspackaged/helpers ^3.0bump applied locally, so it's unrelated to that work. The 8.2 leg on this PR reproduces it exactly.🤖 Generated with Claude Code